-
-
Notifications
You must be signed in to change notification settings - Fork 253
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add lock for thread-safe access to winterm #229
base: master
Are you sure you want to change the base?
Conversation
Hi. Shouldn't the caller be in charge of protecting and synchronizing concurrent write rather than incorporate it into |
I'm not sure if the atomicity of My reasoning was that, since this is a 'compatibility' wrapper, it should try to emulate the observed behaviour of the function being wrapped as close as possible. |
Locking the write calls incurs a performance penalty that I'm not sure we should apply on our level - most applications don't have any problems with thread safety. As @Delgan suggested, maybe when using colorama and having these kinds of threading problems, the caller should apply the locks to synchronize the writes. |
I did think of If each thread called What I'm trying to address here is different though... when wrapping the stream, Also, the performance hit from the lock only applies to Windows, and only when wrapping is enabled. I'd be surprised if a client so concerned with performance would be using All that said, this PR was only an attempt to save your average scripter from surprises now that The only reason I witnessed this behaviour is because I put together a not-so-amazing script in 10 minutes or so, just to batch-execute some HTTP requests... to speed things up, I replaced a simple 2-line loop with |
Hey. FYI, Yesterday I created a PR to test releases before we push them to PyPI. When that is merged, I'll be more confident about resuming merges and releases. I'll try to look at this PR soon. Thank you for creating it! |
Apologies for being AWOL for years, but I'm rounding up PRs that might need merging into Colorama. If you're still keen to champion this, @polys, let me know. It seems useful to me. I don't tend to allow myself to get trapped in threading issues if I can possibly avoid it, so I shan't pretend to be an expert. But also, I'm not terrifically worried about performance overhead until someone demonstrates it's an issue. Having said that, I'd be much more keen to merge this if there was a demonstration of the problem that we could use to justify it, rather than voodoo "I heard it worked on someone's machine once". :-) If that demonstration could be baked in a test, even better. I understand that isn't trivial to do, so would be willing to settle for a human-run demo instead (which I'm not proposing to merge) |
When the output stream is wrapped,
print(...)
is no longer an atomic operation. With multiple threads writing to the console, the text and colours sometimes get a bit scrambled.This PR wraps the write method with a lock to make it atomic.
I have working code that consistently reproduces this and also verifies that the lock resolves the issue but I'm unable to share. I can try to come up with a simpler example that I can share, if needed.